✨ Accept a Secret or a ConfigMap for BMC CA and Trusted CA#526
✨ Accept a Secret or a ConfigMap for BMC CA and Trusted CA#526dtantsur wants to merge 2 commits intometal3-io:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
82a766b to
ef7da9a
Compare
0b36e8f to
8eb848b
Compare
8eb848b to
79832f9
Compare
|
/retest |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #431 by extending the Ironic operator to support both ConfigMaps and Secrets as sources for the BMC CA and Trusted CA certificates. Previously, only Secrets were supported for BMC CA and only ConfigMaps for Trusted CA (mirroring the behavior of the deprecated BMCCAName/TrustedCAName fields). The new BMCCA and TrustedCA structured fields accept either kind, with an additional Key selector for TrustedCA. The deprecated BMCCAName/TrustedCAName fields are retained for backward compatibility.
Changes:
- New
ResourceReferenceandResourceReferenceWithKeytypes added to the API, plus newBMCCAandTrustedCAfields on theTLSstruct, with backward-compatible deprecation of the old fields - Controller, local-mode parser, and container-building logic updated to fetch and mount either a Secret or ConfigMap for each CA, and to select the correct key from a
TrustedCAresource - New
validateCASettingsvalidation function and associated golangci suppression for deprecated field references
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/ironic_types.go |
Adds new BMCCA and TrustedCA fields with deprecation notices on old fields |
api/v1alpha1/common.go |
Defines new ResourceReference and ResourceReferenceWithKey types |
api/v1alpha1/zz_generated.deepcopy.go |
Auto-generated deep-copy methods for new types |
config/crd/bases/ironic.metal3.io_ironics.yaml |
CRD schema updated with new fields and deprecation notices |
docs/api.md |
API documentation updated for new fields |
pkg/ironic/utils.go |
Adds GetBMCCA, GetTrustedCA, volumeForSecretOrConfigMap helpers; extends Resources struct |
pkg/ironic/containers.go |
Updates buildTrustedCAEnvVars to handle both Secret and ConfigMap; updates volume-mounting for both CA types |
pkg/ironic/validation.go |
Adds validateCASettings to validate new fields and their consistency with deprecated ones |
pkg/ironic/version.go |
Extends checkVersion to check for either BMCCASecret or BMCCAConfigMap |
pkg/ironic/local.go |
Updates YAML parsing to load both Secret and ConfigMap for each CA |
internal/controller/ironic_controller.go |
Updates reconciler to fetch Secret or ConfigMap based on resolved CA reference |
pkg/ironic/containers_test.go |
Adds new tests for buildTrustedCAEnvVars with key selection logic |
.golangci.yaml |
Adds staticcheck suppression for deliberate use of deprecated fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Kind: "ConfigMap", | ||
| }, | ||
| Key: "custom-ca.crt", | ||
| }, | ||
| configMapData: map[string]string{ | ||
| "custom-ca.crt": "cert1", | ||
| "other-ca.crt": "cert2", | ||
| }, | ||
| expectedKey: "custom-ca.crt", | ||
| expectWarning: false, | ||
| }, | ||
| { | ||
| name: "Secret with specific key", | ||
| trustedCARef: &metal3api.ResourceReferenceWithKey{ | ||
| ResourceReference: metal3api.ResourceReference{ | ||
| Name: "trusted-ca-secret", | ||
| Kind: "Secret", |
There was a problem hiding this comment.
The test cases use string literals "ConfigMap" and "Secret" for the Kind field rather than the defined constants metal3api.ResourceKindConfigMap and metal3api.ResourceKindSecret. Using the constants would be more consistent with the rest of the codebase (e.g., all production code uses the constants), and would protect against silent breakage if the constant values were ever changed.
pkg/ironic/containers.go
Outdated
| } else { | ||
| cctx.Logger.Info("ignoring duplicate key in Trusted CA ConfigMap", | ||
| "key", key, "configmap", fmt.Sprintf("%s/%s", configMap.Namespace, configMap.Name)) | ||
| cctx.Logger.Info("specified key not found in Trusted CA "+resourceKind+", using first available key", |
There was a problem hiding this comment.
Shouldn't this be validation error, if specifically requsted key isn't present?
There was a problem hiding this comment.
Yeah. I'll try find a way to not pass error all the way into thus function.
Unfortunately, various CA providers are inconsistent in what they create (cert-manager creates secrets, OpenShift creates config maps). This change deprecates BMCCAName and TrustedCAName in favour of BMCCA and TrustedCA that contain a Kind. While here, also address the existing quirk of only supporting the first key of a ConfigMap: now a Key can also be provided. Generated-By: Claude Code (commertical license) Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Assisted-By: Claude Code (commercial license) Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
|
/retest Transient issue |
Unfortunately, various CA providers are inconsistent in what they create (cert-manager creates secrets, OpenShift creates config maps). This change deprecates BMCCAName and TrustedCAName in favour of BMCCA and TrustedCA that contain a Kind.
While here, also address the existing quirk of only supporting the first key of a ConfigMap: now a Key can also be provided.
Generated-By: Claude Code (commertical license)
Closes: #431